fix: improve formbuilder logic #1135

tefkah · 2025-04-03T13:37:08Z

fix: improve client side logic for formbuilder
fix: be able to submit access changes on their own

Issue(s) Resolved

Resolves #1143

High-level Explanation of PR

Roughly does 3 things:

Moves the form builder diff calculation on the client, that way it's easier to only send up changes that are relevant, and we disable the submit button if no changes have been made
Updates access if that is the only thing changed. Previously also the formelements needed to have been moved/deleted/added in order for a change in access to go through
slight beauty enhancements

Moving diff to client

This was put as TODOs in the code in some places, and it made my life easier, so i did.

The main change to the diffing algorithm is here:

platform/core/app/components/FormBuilder/FormBuilder.tsx

Lines 155 to 164 in 23a047b

    
           // check whether the element is reeeaally updated minus the updated field 
        
           const { updated: _, id: _id, ...elementWithoutUpdated } = element; 
        
           const { updated, id, ...rest } = 
        
           	defaultValues.elements.find((e) => e.elementId === element.elementId) ?? {}; 
        
           const defaultElement = rest as Omit<FormElementData, "updated" | "id">; 
        
           if (JSON.stringify(defaultElement) === JSON.stringify(elementWithoutUpdated)) { 
        
           	return acc; 
        
           }

May be a better way of doing this but i thought this was fine for now (i expect the order of keys to be relatively stable).

Basically, if nothing except maybe the id/updatedAt of the form element has changed, we do not consider it a change.

Note

Caveat: A "rank" change is still considered a change. If two elements are swapped back and forth, their rank may still have been changed. I find it hard to check for this case as i'll admit i don't fully understand the ranking implementation, maybe @kalilsn you have some insight on how to ignore swapping back and forth cases.

Warning

Moving the diffs to the client now does carry slightly more risk for weird stuff happening, but not really that much more than before

Update logic changes

Since all the changes now arrive as separate entities, we can more easily only update form accesstype if that has changed.

QoL changes

When hovering over the drag handle, we now show cursor-drag rather than cursor-pointer
When dragging, we now show cursor-dragging, rather than cursor-pointer
When dragging, the dragged element no longer sometimes slides under other elements when dragged.

Demo:
(slightly hard to see bc recording my screen apparently makes the cursor almost always be the default one)

Screen.Recording.2025-04-14.at.12.14.28.mov

Test Plan

Screenshots (if applicable)

Notes

…nt does not 'duck' under existing ones

tefkah · 2025-04-14T10:42:53Z

core/app/c/[communitySlug]/forms/[formSlug]/edit/page.tsx

-	searchParams: Promise<{
-		unsavedChanges: boolean;
-	}>;
 }) {
-	const searchParams = await props.searchParams;
-
-	const { unsavedChanges } = searchParams;
-


this logic does not need to happen server side, we can just do it client side. this is much faster

tefkah · 2025-04-14T10:43:26Z

core/app/components/FormBuilder/FormBuilder.tsx

+					if (JSON.stringify(defaultElement) === JSON.stringify(elementWithoutUpdated)) {
+						return acc;
+					}


very sophisticated diffing. please recommend a better approach if you know one

tefkah · 2025-04-14T10:44:57Z

core/app/components/FormBuilder/FormBuilder.tsx

-	const form = useForm<FormBuilderSchema>({
-		resolver: zodResolver(formBuilderSchema),
-		values: {
+	const [isChanged, setIsChanged] = useIsChanged();


this is a small custom nuqs hook reads from/updates the isChanged query param. using nuqs we can do this without a round trip to the server, which we want. this is because shallow is set to true by default. maybe i should add that option to make it slightly more clear that these are client-side only url updates

platform/core/app/components/FormBuilder/useIsChanged.tsx

Lines 1 to 13 in 415de67

import { parseAsBoolean, useQueryState } from "nuqs";

export const useIsChanged = () => {

const [isChanged, setIsChanged] = useQueryState(

"unsavedChanges",

parseAsBoolean.withDefault(false).withOptions({

history: "replace",

scroll: false,

})

);

return [isChanged, setIsChanged] as const;

};

tefkah · 2025-04-14T10:47:16Z

core/app/components/FormBuilder/SaveFormButton.tsx

@@ -18,7 +21,7 @@ export const SaveFormButton = ({ form, className, disabled }: Props) => {
 			form={form}
 			type="submit"
 			data-testid="save-form-button"
-			disabled={disabled}
+			disabled={disabled != null ? disabled : !isChanged}


maybe i should just remov the disabled prop entirely and make it !isChanged by default

tefkah · 2025-04-14T10:50:14Z

core/app/components/FormBuilder/FormBuilder.tsx

+						if (
+							element.relatedPubTypes.length === 0 &&
+							defaultElement.relatedPubTypes?.length
+						) {


i added this defaultElement.relatedPubTypes?.length check, bc otherwise if you swapped a relation element back and forth it would assume something had to be deleted, which didn't make sense

allisonking

woo this is awesome! approving, though I don't know about the ranking part either though, so maybe wait for Kalil to weigh in on that part?

tefkah added 3 commits April 3, 2025 15:35

fix: improve client side logic for formbuilder

415de67

fix: be able to submit access changes on their own

a4f40ec

fix: remove passing of disabled prop manually

aff5b58

tefkah mentioned this pull request Apr 5, 2025

Bug: cannot make form public #1143

Closed

tefkah added 7 commits April 14, 2025 11:22

chore: merge

7880ec8

test: add test for changing access

fecbf8f

fix: be slightly more granular with deleted relatedpubtype updates

f0d9c71

fix: add grabbing state to cursor, add z-index when grabbing so eleme…

23a047b

…nt does not 'duck' under existing ones

fix: use same transaction for updating relatedPubTypes

d4189a9

test: fix tests

89d5e4f

fix: do not run ranking test

da15ed7

tefkah commented Apr 14, 2025

View reviewed changes

tefkah requested review from allisonking and kalilsn April 14, 2025 10:50

tefkah marked this pull request as ready for review April 14, 2025 10:51

Merge branch 'main' into tfk/formbuilder-fixes

d64b132

allisonking approved these changes Apr 15, 2025

View reviewed changes

tefkah and others added 3 commits April 15, 2025 17:02

Merge branch 'main' into tfk/formbuilder-fixes

d17abce

Merge branch 'main' into tfk/formbuilder-fixes

c72760b

Merge branch 'main' into tfk/formbuilder-fixes

57ffa98

3mcd merged commit bfd67ba into main Apr 22, 2025
16 checks passed

3mcd deleted the tfk/formbuilder-fixes branch April 22, 2025 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: improve formbuilder logic #1135

fix: improve formbuilder logic #1135

tefkah commented Apr 3, 2025 •

edited

Loading

tefkah Apr 14, 2025

tefkah Apr 14, 2025

tefkah Apr 14, 2025 •

edited

Loading

tefkah Apr 14, 2025

tefkah Apr 14, 2025

allisonking left a comment

	// check whether the element is reeeaally updated minus the updated field
	const { updated: _, id: _id, ...elementWithoutUpdated } = element;
	const { updated, id, ...rest } =
	defaultValues.elements.find((e) => e.elementId === element.elementId) ?? {};

	const defaultElement = rest as Omit<FormElementData, "updated" \| "id">;

	if (JSON.stringify(defaultElement) === JSON.stringify(elementWithoutUpdated)) {
	return acc;
	}

	import { parseAsBoolean, useQueryState } from "nuqs";

	export const useIsChanged = () => {
	const [isChanged, setIsChanged] = useQueryState(
	"unsavedChanges",
	parseAsBoolean.withDefault(false).withOptions({
	history: "replace",
	scroll: false,
	})
	);

	return [isChanged, setIsChanged] as const;
	};

fix: improve formbuilder logic #1135

fix: improve formbuilder logic #1135

Conversation

tefkah commented Apr 3, 2025 • edited Loading

Issue(s) Resolved

High-level Explanation of PR

Moving diff to client

Update logic changes

QoL changes

Test Plan

Screenshots (if applicable)

Notes

tefkah Apr 14, 2025

Choose a reason for hiding this comment

tefkah Apr 14, 2025

Choose a reason for hiding this comment

tefkah Apr 14, 2025 • edited Loading

Choose a reason for hiding this comment

tefkah Apr 14, 2025

Choose a reason for hiding this comment

tefkah Apr 14, 2025

Choose a reason for hiding this comment

allisonking left a comment

Choose a reason for hiding this comment

tefkah commented Apr 3, 2025 •

edited

Loading

tefkah Apr 14, 2025 •

edited

Loading